-
-
Notifications
You must be signed in to change notification settings - Fork 58
feat(prefer-svelte-reactivity): reporting returned variables #1326
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(prefer-svelte-reactivity): reporting returned variables #1326
Conversation
🦋 Changeset detectedLatest commit: 1f6450e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Try the Instant Preview in Online PlaygroundInstall the Instant Preview to Your Local
Published Instant Preview Packages:
|
0441041
to
a299b31
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should we be forced to use SvelteSet
or SvelteMap
even when the values aren’t changing?
Firstly, this PR is part of a wider effort to move the rule to better detect when variables are "encapsulated" inside a function/class and not report them - this PR is an off-shoot of #1287 But this should make sense even on its own anyway - but I think it does. Example
function makeSet() {
const a = new Set();
// Populate the set etc.
return a;
}
export makeSet;
<script>
import { makeSet } from "fileA";
const a = makeSet();
</script>
<!-- Print the set here -->
<button onclick={a.add(42);}>Click me!</button> In this example, this PR reports the incorrect usage of |
5774d2f
to
4f23b63
Compare
4f23b63
to
22b93d5
Compare
22b93d5
to
1f6450e
Compare
} | ||
</script> | ||
|
||
{fn().has(42)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. This is actually fine as is, right? (It shouldn't be reported, right?)
Could you change it to track the value the function returns, check if there's a mutation, and only report that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking about this more and you are right. I think this needs a change of approach. I will probably make an alternative PR.
No description provided.